Classify -32022 and SSE backends as Legacy, sync stale comments by jhrozek · Pull Request #5997 · stacklok/toolhive

jhrozek · 2026-07-25T19:52:37Z

Summary

Review follow-ups for #5993, stacked on adopt-go-sdk-v1.7 (base is that branch, not main).

#5993 correctly moved backend revision classification onto server/discover's supportedVersions. Reviewing it turned up four further problems in the same area — two where the same "is this peer Modern?" question was still answered by the old rule, plus a one-directional cache and a spec-MUST gap:

A -32022 was treated as proof of Modern. It means the opposite — the peer does not support the version we asked for. A backend answering it got cached Modern permanently and enumerated zero capabilities, silently: modernListCapabilities tolerates the error as caps == nil, and isRevisionMismatch never reclassified it. Now the error's data.supported list decides, keeping it Modern-positive only when 2026-07-28 is still advertised. This mirrors go-sdk's own reference client (mcp/client.go), whose test for the case is named "unsupported protocol version falls back to initialize". -32020/-32021 remain unconditionally Modern — both require the peer to have parsed Modern _meta.
probeRevision was transport-blind. TransportType == "sse" names the deprecated 2024-11-05 two-endpoint transport, whose BaseURL is the GET-only /sse path (pkg/transport/url.go:56-57, httpsse/http_proxy.go:274-278), and modernCall is a single-endpoint POST — so no Modern endpoint is reachable there. It now classifies Legacy without probing, which also avoids POSTing into a stream and hanging to the client timeout (errModernTransient is uncached, so that cost recurred on every call).
The revision cache only self-healed one way. A backend cached Modern that had negotiated down recovered via reclassify; one cached Legacy that had become Modern never did, because go-sdk v1.7 negotiates Modern transparently on the Legacy path so the reclassify-on-error trigger never fires. That stale value is republished into the mcpRevision CRD status field every health tick. Fixed in-band: initializeClient already received the negotiated version and discarded it, and every Legacy dispatch builds a fresh client — so the mis-routed call itself re-negotiates and the flip costs no extra round trip. Closes the Legacy→Modern half of vMCP backend revision cache has no success/TTL re-probe (stale on stateless redeploy) #5992.
Mcp-Name was never sentinel-encoded, though the spec makes it a client MUST for values not safely representable as plain ASCII, and for plain-ASCII values matching the sentinel pattern. decodeSentinelName existed with no counterpart; a URI containing CR/LF makes Go's transport reject the request outright.

The rest is comment accuracy and test coverage. Several comments contradicted the code after the v1.7 bump, which .claude/rules/go-style.md ("Keep Comments Synchronized With Code") calls out as worse than no comment — including five that justified the hand-rolled Modern shim on the grounds that go-sdk v1.6.1 cannot express the Modern wire shape, the premise #5993 invalidates.

Type of change

Test plan

Unit tests (task test) — targeted runs of ./pkg/vmcp/client/..., ./pkg/vmcp/session/..., ./pkg/mcp/..., all green
E2E tests (task test-e2e)
Linting (task lint-fix) — 0 issues
Manual testing (describe below)

New coverage: TestProbeRevision_SSEGate (handler fails the test if hit, proving no network call), both -32022 branches in TestProbeRevision_TruthTable, two isRevisionMismatch truth-table rows, TestHTTPSession_CallTool_StripsReservedModernMeta for the previously untested session-path strip site, TestLegacyInit_SelfHealsRevisionCache plus a flipped assertion in the renamed TestListCapabilities_MisCachedLegacy_SelfHealsViaSDKNegotiation, and TestEncodeSentinelName (round-tripping every case through decodeSentinelName).

API Compatibility

This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File	Change
`pkg/vmcp/client/modern.go`	`classifyUnsupportedProtocolVersion` decodes `data.supported`; `Data` added to `modernRPCError`; `-32022` dropped from `isModernProtocolCode`
`pkg/vmcp/client/client.go`	SSE⇒Legacy gate in `probeRevision`; `initializeClient` returns the negotiated version and `legacyInit` flips a stale Legacy cache; revision-cache and TRIPWIRE docs corrected
`pkg/vmcp/client/revision_test.go`	SSE-gate test; `-32022` split into with/without `data.supported`
`pkg/vmcp/server/{modern_envelope,modern_dispatch,serve}.go`	Stale `v1.6.1` shim rationales corrected
`pkg/mcp/parser_integration_test.go`	Wrong SSE explanation replaced with the real mechanism, now asserted
`pkg/mcp/revision.go`	`EncodeSentinelName` mirroring `decodeSentinelName`; wired into the `Mcp-Name` header
`pkg/vmcp/client/reclassify_test.go`	`errModernNegotiatedDown` truth-table rows; staleness doc amended
`pkg/vmcp/session/internal/backend/*_test.go`	Session-path strip test; `metaFor` → `toolCallMeta`
`pkg/vmcp/client/{client_test,schema_ingestion_regression_test}.go`	Fail-loudly on dropped fake response; pin the Legacy path

Does this introduce a user-facing change?

Yes, three things. A backend that rejects our Modern probe with -32022, and any backend on the sse transport, is now correctly driven as Legacy (2025-11-25) — previously a -32022 backend was driven Modern and advertised no tools, resources, or prompts, so affected backends go from silently empty to working. A backend redeployed stateful→stateless now reports the correct mcpRevision in VirtualMCPServer status instead of 2025-11-25 indefinitely. And a tool name or resource URI that is not plain ASCII is now sent correctly on the Modern egress rather than raw.

Special notes for reviewers

Retry safety for -32022. errModernNegotiatedDown feeds isRevisionMismatch, so dispatch reclassifies and retries — and unlike the discover source, this one can fire on a side-effecting tools/call. It is safe: go-sdk's ServerSession.handle returns -32022 before the switch req.Method that dispatches to a handler, so the backend provably never executed the request. The sentinel's doc previously justified this as "both call sites are read-only", which is no longer true; it now states the real reason.
Exact-match vs range. go-sdk retries Modern on any negotiated version >= 2026-07-28; we exact-match MCPVersionModern, because vMCP's shim speaks exactly one Modern wire shape. Safe today (a dual-era peer also lists 2025-11-25 to fall back to), and this is now the second exact-match site, so the existing TRIPWIRE note names both.
HTTP+SSE is deprecated, not removed. An earlier draft of the SSE gate justified it as "2026-07-28 removed HTTP+SSE". That is wrong — the spec's deprecation page states no features have been removed yet; what 2026-07-28 removed is Streamable HTTP's GET stream and sessions. The gate is justified on ToolHive routing instead. Please don't "correct" it back.
Apparent contradiction, and why it isn't one. parser_integration_test.go observes Modern negotiation over an sse-labelled endpoint while probeRevision now asserts that can never happen. Different layers: mcpcompat's SSE client reaches /messages, whereas vMCP's modernCall only ever POSTs to BaseURL, which is /sse.
Deliberately not fixed here (follow-up issue to be filed once Support MCP 2026-07-28 spec (go-sdk v1.7 via toolhive-core) #5993 lands): the Legacy→Modern revision cache never self-heals under v1.7, and the stale value reaches the mcpRevision CRD status field (vMCP backend revision cache has no success/TTL re-probe (stale on stateless redeploy) #5992). A promising in-band fix exists — initializeClient already receives InitializeResult.ProtocolVersion and discards it. Also pending: io.modelcontextprotocol/logLevel (confirmed a real fourth reserved per-request key, but deferred deliberately — ReservedModernMetaKeys currently does double duty as the ingress Modern-signal set, so appending it would change request classification and start rejecting requests that work today, for a field SEP-2577 already deprecates), Mcp-Name sentinel encoding, x-mcp-header mirroring, and arch docs for the whole dual-revision subsystem (still undocumented).
Commits are split behavioral / comments / tests and are worth reading in order.

Generated with Claude Code

codecov · 2026-07-25T20:01:42Z

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.11%. Comparing base (574d3f8) to head (8ecf864).

Files with missing lines	Patch %	Lines
pkg/vmcp/client/modern.go	93.33%	1 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           adopt-go-sdk-v1.7    #5997      +/-   ##
=====================================================
- Coverage              72.13%   72.11%   -0.02%     
=====================================================
  Files                    718      718              
  Lines                  74480    74504      +24     
=====================================================
+ Hits                   53724    53729       +5     
- Misses                 16908    16923      +15     
- Partials                3848     3852       +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JAORMX

Reviewed the full diff (base is adopt-go-sdk-v1.7, not main). Both behavioral fixes are correct and the reasoning holds up:

-32022 classification. Dropping it from isModernProtocolCode and routing it through classifyUnsupportedProtocolVersion is right — a bare -32022 means "I don't support the version you asked for," which a peer negotiating down to Legacy also returns, so treating it as unconditional proof-of-Modern was the bug (silently cached Modern + enumerated zero capabilities). Keying the decision on data.supported containing MCPVersionModern, with best-effort decode → empty → negotiated-down, is the correct fail-safe direction. -32020/-32021 staying unconditionally Modern is right (both require the peer to have parsed Modern _meta).
SSE gate in probeRevision. Classifying TransportType == "sse" (the 2024-11-05 two-endpoint transport, GET-only /sse BaseURL) as Legacy without probing is correct — modernCall is a single-endpoint POST that can't reach a Modern endpoint there — and it avoids POSTing into a GET stream and eating the client timeout on every call (since errModernTransient is uncached). TestProbeRevision_SSEGate proves no network call by failing if the handler is hit.

The retry-safety property I paid the most attention to: errModernNegotiatedDown now feeds isRevisionMismatch, so dispatch reclassifies and retries — and via interpretModernResult this can fire on a side-effecting tools/call, unlike the discover-sourced case. The no-double-execution guarantee rests entirely on the peer rejecting the unsupported protocol version before executing the method. That's correct for a spec-compliant peer (per-request version validation precedes method dispatch, as in go-sdk's ServerSession.handle), and you've documented it precisely on the sentinel. Worth being explicit that this is the load-bearing assumption for any non-go-sdk Modern backend — but it's the right call and consistent with the double-exec discipline in the Legacy path.

Comment re-syncs are accurate (the stale v1.6.1-can't-express rationales, the revision-cache asymmetric-self-heal note pointing at #5992, the exact-match TRIPWIRE now naming both sites), and the new session-path strip test + toolCallMeta rename are good. Nice catch on the whole class of "old rule answering the same question" bugs.

Non-blocking: the #5992 staleness (mis-cached Legexpiry→Modern never self-heals and leaks to the mcpRevision CRD field) is real and I agree it's out of scope here — the initializeClient InitializeResult.ProtocolVersion in-band fix you mention sounds like the right follow-up.

Two backend-revision signals disagreed with the supportedVersions rule adopted for go-sdk v1.7, each able to strand a backend on the wrong path. A -32022 (CodeUnsupportedProtocolVersion) was treated as proof of Modern. It means the opposite: the peer does not support the version we asked for. A backend answering it was cached Modern permanently and enumerated zero capabilities, since isRevisionMismatch never reclassified it. Decode the error's data.supported list instead and keep it Modern-positive only when 2026-07-28 is still advertised, mirroring go-sdk's own reference client (mcp/client.go), whose test for this case is named "unsupported protocol version falls back to initialize". -32020/-32021 stay unconditionally Modern: both require the peer to have parsed Modern _meta. probeRevision was also transport-blind. TransportType "sse" names the deprecated 2024-11-05 two-endpoint transport, whose BaseURL is the GET-only /sse path, and modernCall is a single-endpoint POST, so no Modern endpoint is reachable there. Classify it Legacy without a probe, which also avoids POSTing into a stream and hanging to the client timeout (errModernTransient is uncached, so that cost recurred on every call). Retrying under the corrected Legacy classification stays safe for tools/call: go-sdk rejects an unsupported per-request protocol version before dispatching to any method handler, so the backend never executed the request. Also record that revision-cache self-healing is asymmetric under v1.7 and that the stale value reaches the mcpRevision CRD field (#5992). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Five comments justified the hand-rolled Modern shim on the grounds that go-sdk v1.6.1 cannot express the Modern wire shape. The repo is now on v1.7.0-pre.3, which implements the revision, so that premise reads as false to anyone who checks it. The shim is still required, but for a narrower reason: mcpcompat's public Client API has no no-initialize primitive, only the private Legacy-shaped resumeCall. Say that instead. The parser integration test explained its SSE behavior by claiming the harness cannot capture an initialize carried over the SSE message channel. That was self-contradictory, since tools/call and resources/read traverse the same channel and the same middleware and are captured. The real mechanism: mcpcompat's SSE server uses go-sdk's NewSSEHandler, whose transport implements no ProtocolVersionSupporter gate, so it advertises 2026-07-28, discover succeeds, and initialize is never sent on that arm. Assert that directly rather than inferring it, and drop the claim that the streamable arm sends no discover -- it does, and is negotiated down. Note also that HTTP+SSE is deprecated but NOT removed by 2026-07-28; what that revision removed is Streamable HTTP's GET stream and sessions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The errModernNegotiatedDown arm of isRevisionMismatch had no truth-table row despite being the only surviving self-healing direction under go-sdk v1.7, so add both the Modern (mismatch) and Legacy (not a mismatch) cases. The reserved-_meta strip has two call sites but only the httpBackendClient one was tested; add the persistent-session counterpart, asserting reserved keys are stripped, non-reserved caller keys survive, and the caller's map is not mutated. Its fixture uses scalars because maps.Clone is shallow and would not have caught a nested mutation. Two robustness fixes: the SSE fake dropped a queued response after a five second timeout and returned an empty body, turning a failure into a hang against a deadline-free context, so fail loudly instead (both send sites, not just one). And the schema-fidelity test reached its Legacy path only because the fake's catch-all happens to answer discover with a body that lacks resultType; pin the revision so adding a field to that arm cannot silently route the test through the Modern shim and strand the injected clientFactory it exists to exercise. metaFor loses its method parameter (every caller passed tools/call, and a second calling function tripped unparam) and becomes toolCallMeta. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The revision cache healed in only one direction. A backend cached Modern that had negotiated down to Legacy recovered via reclassify, but one cached Legacy that had become Modern never did: go-sdk v1.7 negotiates Modern transparently on the Legacy dispatch path, so the call succeeds and the reclassify-on-error trigger never fires. The stale value is not internal bookkeeping — CachedRevision feeds health status and is republished into the mcpRevision CRD field on every tick, so a backend redeployed stateful to stateless reported 2025-11-25 indefinitely. Read the negotiated version off the initialize result instead of waiting for an error that never comes. initializeClient already received it and discarded it, and every Legacy dispatch builds a fresh client, so the mis-routed call itself re-negotiates and the flip costs no extra round trip. The value is genuine on both SDK paths: Connect() stores the discover-negotiated version, and the fallback stores the real initialize response. A backend without server/discover can never yield 2026-07-28, so this cannot promote a genuinely Legacy peer. Closes the Legacy-to-Modern half of #5992; the TTL re-probe now only has to cover whatever remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Two small conformance and duplication fixes on the Modern egress. The 2026-07-28 spec requires a client to Base64 sentinel-encode an Mcp-Name value that cannot be safely represented as a plain ASCII header, and to encode any plain-ASCII value that already matches the sentinel pattern so a server does not wrongly decode it. We sent the value raw, with only a comment acknowledging the gap. A non-ASCII tool name reaches a strict peer as unspecified behavior, and a URI containing CR or LF makes Go's transport reject the request outright. decodeSentinelName already existed with no counterpart, so add its exact mirror and use it. mergeModernMeta also hand-rolled the same maps.Clone plus delete loop that StripReservedModernMeta now performs. Two copies of one rule will drift as soon as a fourth reserved key is added, which is a live prospect -- io.modelcontextprotocol/logLevel is a real per-request reserved key we are deliberately deferring, because the key list currently does double duty as the ingress Modern-signal set and enrolling it there would change request classification. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

JAORMX

Re-reviewed the full diff at 6151ef10 (base is adopt-go-sdk-v1.7, not main). The force-push since my earlier review grew this from 2 fixes to 4; the two carried-over fixes still hold and the two new ones are correct. LGTM.

Carried over — still correct:

-32022 classification. Dropping it from isModernProtocolCode and routing it through classifyUnsupportedProtocolVersion (decode data.supported; Modern-positive only if it still lists MCPVersionModern, else errModernNegotiatedDown; best-effort decode → empty → negotiated-down) is the right fail-safe direction, and the two new truth-table rows (-32022 with vs. without data.supported) pin both branches. -32020/-32021 stay unconditionally Modern — correct.
SSE gate in probeRevision. TransportType == "sse" (the deprecated 2024-11-05 GET-/sse transport) → Legacy without a probe is correct — modernCall is a single-endpoint POST that can't reach a Modern endpoint there — and TestProbeRevision_SSEGate fails if the handler is hit, proving no network call.

New — reviewed in depth:

Legacy→Modern cache self-heal (legacyInit/initializeClient). Returning InitializeResult.ProtocolVersion and flipping the cache to Modern when it equals MCPVersionModern is sound: the value is the genuinely-negotiated version, so a Legacy (2025-11-25) backend never trips it, and a mis-cached-Legacy-but-actually-Modern backend now heals in-band instead of never (since go-sdk v1.7 negotiates Modern transparently on the Legacy path, the reclassify-on-error trigger never fires). No double-execution risk — initialize is read-only, and the flip doesn't affect the in-flight call. Closes the Legacy→Modern half of #5992, and the health-status/mcpRevision CRD-field staleness you call out is a real operator-visible bug, not just bookkeeping. TestLegacyInit_SelfHealsRevisionCache + the flipped TestListCapabilities_MisCachedLegacy_SelfHealsViaSDKNegotiation cover it.
Mcp-Name sentinel encoding. EncodeSentinelName is a faithful mirror of decodeSentinelName — encodes when any byte is outside 0x21–0x7E (so CR/LF/whitespace/non-ASCII) or when the value already looks like a sentinel, else passes through unchanged. Beyond the spec-MUST, this also closes a real hardening gap: a CR/LF in a tool name or resource URI would previously make net/http reject the request outright. TestEncodeSentinelName round-trips every case (plain ASCII + URI unchanged; accented/CJK/CR/LF/sentinel-shaped encoded) back through decodeSentinelName. Good.

One assumption worth restating (not blocking): errModernNegotiatedDown from -32022 now feeds isRevisionMismatch, so dispatch can reclassify+retry a side-effecting tools/call. Safety rests entirely on a spec-compliant peer rejecting the unsupported protocol version before dispatching to the method handler (as go-sdk's ServerSession.handle does). That's the right call and you've documented it precisely on the sentinel, but it remains the load-bearing invariant for any non-go-sdk Modern backend.

Comment re-syncs (v1.6.1 → v1.7.0-pre.3 / mcpcompat rationales, the exact-match TRIPWIRE now naming both sites, the both-ways self-heal note on the revisions field) are accurate, and the mergeModernMeta → StripReservedModernMeta dedup with the nil-guard is clean.

CI note: at time of review the run on 6151ef10 is green across unit/lint/docs/security/operator/conformance/all three E2E Test Lifecycle + all E2E-core shards except E2E Tests Core (core), which is still in progress (no failures). Approving on the code; this targets a feature branch so it isn't a merge-to-main candidate regardless.

Adopt the MCP 2026-07-28 ("Modern"/stateless) spec revision via toolhive-core v0.0.34 (go-sdk v1.7.0-pre.3), so vMCP negotiates and speaks both revisions per backend (Legacy 2025-11-25 + Modern 2026-07-28) without regressing Legacy. - Revision classification keyed on server/discover supportedVersions (SEP-2575), with errModernNegotiatedDown routing negotiate-down through reclassify. - Reserved io.modelcontextprotocol/* _meta stripped at Legacy backend egress. - Includes #5997 fixes (-32022 and SSE targets classified Legacy; stale-comment sync) and the reserved-meta strip/signal two-list split (+logLevel). Follow-ups: #5992 (revision-cache staleness), #6002 (Modern-egress logLevel doc/test).

jhrozek requested review from ChrisJBurns, JAORMX, amirejaz, blkt, jerm-dro, rdimitrov and tgrunnagle as code owners July 25, 2026 19:52

github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jul 25, 2026

jhrozek mentioned this pull request Jul 25, 2026

Support MCP 2026-07-28 spec (go-sdk v1.7 via toolhive-core) #5993

Merged

3 tasks

JAORMX approved these changes Jul 25, 2026

View reviewed changes

JAORMX force-pushed the adopt-go-sdk-v1.7 branch from ca1198a to 574d3f8 Compare July 27, 2026 06:24

JAORMX requested a review from reyortiz3 as a code owner July 27, 2026 06:24

jhrozek and others added 3 commits July 27, 2026 09:14

jhrozek force-pushed the fix/pr5993-review-followups branch from acc3e5d to 6151ef1 Compare July 27, 2026 07:15

github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 27, 2026

jhrozek and others added 2 commits July 27, 2026 09:44

github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 27, 2026

JAORMX approved these changes Jul 27, 2026

View reviewed changes

jhrozek mentioned this pull request Jul 27, 2026

MCP 2026-07-28 conformance and revision follow-ups from the go-sdk v1.7 review #6002

Closed

jhrozek merged commit 4d0fe92 into adopt-go-sdk-v1.7 Jul 27, 2026
75 of 77 checks passed

jhrozek deleted the fix/pr5993-review-followups branch July 27, 2026 08:30

jhrozek mentioned this pull request Jul 27, 2026

Refuse an unserved MCP revision conformantly #6009

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Classify -32022 and SSE backends as Legacy, sync stale comments - #5997

Classify -32022 and SSE backends as Legacy, sync stale comments#5997
jhrozek merged 5 commits into
adopt-go-sdk-v1.7from
fix/pr5993-review-followups

jhrozek commented Jul 25, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 25, 2026 •

edited

Loading

Uh oh!

JAORMX left a comment

Uh oh!

JAORMX left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jhrozek commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

API Compatibility

Changes

Does this introduce a user-facing change?

Special notes for reviewers

Uh oh!

codecov Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JAORMX left a comment

Choose a reason for hiding this comment

Uh oh!

JAORMX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jhrozek commented Jul 25, 2026 •

edited

Loading

codecov Bot commented Jul 25, 2026 •

edited

Loading